PR: Add missing imports and modules #344
Conversation
|
Hi @DaelonSuzuka thank you for checking into this and submiting an initial fix! Regarding the missing imports I would say that was caused due to those modules not being available when the PR for adding support for PySide6 was being implemented (I think that was between when PySide6 6.1.3 was the latest release and PySide6 6.2.0 was going to be released). So, for example, if you try to import
However if those modules are now available then we should add them 👍 Would you like to add all the missing imports on this PR @DaelonSuzuka ? Also, it would be nice then to revisit the test skips to for those modules (to run them againts PySide6/PyQt6). For example, the current test for qtpy/qtpy/tests/test_qtwebsockets.py Lines 4 to 7 in ccf308a Regarding the other elements (files standarizartion and checks for PySide6-Essentials and PySide6-Addon), could you create new issues to discuss them please? Thank you again! |
This raises an important question: What versions of Qt libraries is QtPy committed to supporting? Only the most recent version of each of the 4, or should it correctly work with any version you can get from PyPA? Some users might have a reason to stick to an older version of Qt, is that QtPy's problem or their problem?
I can give it a shot. I'll try to have it in sometime this week. |
Currently for Qt5/PyQt5 the minimum is 5.9, for PySide2 is 5.12 and for Qt6 I think we came to the conclusion to enforce using Qt 6.2.0+ (PyQt6 and PySide6). The minimum versions definitions are here: Lines 93 to 96 in 3c6bd90 For any lower version we are currently raising a warning (basically suggesting to update to a more recent version and saying that we don't support such version). The point where that is being validated is here: Lines 229 to 255 in 3c6bd90 |
|
Thanks for the references. This looks relatively straightforward. |
|
Awesome! If you have any further questions or need help let us know 👍 |
Thanks, I'd been hoping to do the same for some time now. I've opened #345 to track this. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
DaelonSuzuka
force-pushed
the
patch-1
branch
2 times, most recently
from
ccf308a
to
8781e9a
Compare
|
I spent a bunch of time trying to cross-reference the module lists in each library's documentation, and then I remembered that I'm a programmer. So, I created a venv, installed all 4 qt libs, scraped them for files named The following table is every module in PyQt5, PyQt6, PySide2, and PySide6, along with which libraries each module is present in. Modules that are currently missing from qtpy are marked with a star. Modules[1] QtCharts is known to be missing from PyQt5 and PyQt6, requiring a seperate pip install Missing Modules OnlyAre there any objections to my adding the missing modules? Additionally, I don't know why the |
|
Wow, thanks for all the work @DaelonSuzuka ! That's awesome!
So long as they are included in all supported versions of each binding (5.9, 5.12, 5.15 and 6.2) on each platform or have the appropriate conditionals, and are added to our tests (which should mostly verify that), I don't see why not, but maybe @dalthviz has more insight.
I'm guessing because they are a separate PyPI package like QtCharts? |
Awesome, thanks. I searched PyPI for exactly this but didn't get any hits. |
|
Wow. Riverbank doesn't seem to have a list of all their packages, and I can't search PyPI by author to make sure I have them all. Very frustrating. This list is all the PyQt packages I was able to identify. PyQt PackagesWith the above packages installed, the module report makes a lot more sense: Module Report |
Searching for "riverbank" shows all of them with a handful of false positives, and viewing Phil Thompson's profile displays what should be all of them more precisely. |
|
Thanks for taking the time to review all the missing imports @DaelonSuzuka and @CAM-Gerlach for giving feedback ! Just as a side note, probably for missing imports of modules that are packaged independently we should do something similar to what is done for QtCharts: Lines 12 to 29 in 3c6bd90 |
|
Indeed, and it would be helpful to include the package name to install in such messages, given @DaelonSuzuka 's difficulties finding it |
I think I was expecting to be able to navigate directly from the PyQt5/6 package to a page of that account's uploads or something, and I guess I gave up too soon. Thanks for the double check (again).
@dalthviz I was planning on raising a
This has turned out to be a bigger job than I expected, and it's a waste of time to half-ass it, so thanks to both of you for your patience. Hopefully I'll have more time this week and I can get this knocked out, |
Oh, did just clicking on the maintainer's account name on the left of the package page not work for you?
Thanks for all your help and hard work! Though, don't forget (as I often do...)—"the perfect is the enemy of the great," as they say... |
|
Wow, that's a lot of failing test output. I think it's all because of the I have to admit I don't have that much experience writing tests, but the structure of the test suite kind of looks like a code smell to me. It feels like it's testing the implementation more than the interface, I don't really have a proposal except for my gut reaction of generating the tests from the Qt libraries(probably not the right thing to do), but I wanted to mention it anyways. Regardless, I'm wiped for tonight, so I'll start on these tomorrow. I think one more evening will finish off this PR.
I have definitely fallen prey to that impulse, but in this case I'm just slow because I was sick last week and I'm catching up on all the work I slept through. |
|
Oh, I almost forgot to mention, here and here are all of the new Exception messages. I would definitely appreciate a proofread, and if you see a better way to phrase any of them I'll be happy to change it. Edit: I forgot the package names for the external PyQt libraries. Definitely open to suggestions for how to phrase those. |
CAM-Gerlach
changed the title
There was a problem hiding this comment.
Wow, @DaelonSuzuka , that's indeed a lot of work—thanks! Its a sufficiently big extension of QtPy (with many new modules, new errors, and other changes) that I think it makes sense to ship it in 1.2, assuming @dalthviz agrees.
I highly recommend applying my GitHub suggestions first, which are generally straightforward and uncontroversial, before addressing the higher level comments and feedback, assuming you don't disagree with them; otherwise, they are likely to get outdated/lost.
Some high-level feedback and suggestions:
-
Right now, since QtPy is still using the implicit ("flat") package layout rather than the now-recommended explicit
srclayout, this means thatmodule_reportis a top-level import package equal toqtpyitself and will be included in the distribution package and installed in the user's environment as a separate import package, which is of course not what we want. We could exclude it with some futzing in the setuptools configuration, but IMO its still confusing to have two top-level import packages, one of which is just a development-focused helper tool.Therefore, after consultation with you and @dalthviz , I suggest we do one of the following (there is a third option, making it part of the
qtpypackage itself, but I don't think that is more compelling than either of the other two for now):- Stick
module_reportinside atoolsdirectory (which wouldn't be importable and is ignored by default in the new Setuptools 61 auto-discovery) and run it from there when needed (simplest solution to the immediate problem) - Make
module_reportits own separate repo/distribution package (Better usability for us and others without bundling it in QtPy itself, and if you want to help maintain it, you could much more easily be added as a Collaborator on that repo, but is more initial work, more overhead and less directly coupled to QtPy's own development)
Making it a separate repo, installable (at least) from GitHub, is the "nicest" solution, but also the most work, up front and (to some extent) over time, so whether this is viable depends on whether you want to help maintain it, and also on if it more generally useful to application developers rather than just development of QtPy itself. So IMO, just keeping it in a
toolsdirectory is probably the best approach for now unless and until those other conditions are a compelling enough reason to do otherwise (which we could always do in the future). - Stick
-
If we're going to check the output files into the repo, we should run
module_reportin our CIs to catch any changes and either update the files or take other action accordingly. However, given the results can vary between OSes (e.g.DBuson Windows) and distributions (pip/conda), it might be better to just not check them in directly, or if so make the checked-in files specifically for comparison to a per-platform CI check that determines if the output matches the expected (but that latter part would be optional for this PR). This could also help automate and decouple the testing from the details of the implementation, which you brought up some valid concerns about—but again, that could go in a future PR. -
Instead of making all the exception messages base
PythonQtErrors (such that there's no way for tools to programmatically determine whether the module was not installed, not supported on that binding, or some other error occurred) and writing manual messages for each one (which is way more work to add, check and change, and easier to make a typo on), we should subclass it with specific errors of each type, that take anameand abindingargument (with amsgoverride) and construct the message from there. This also allows us to have it inherit the built-inModuleNotFoundErrorso it behaves as expected.Therefore, I suggest adding the following to the
__init__.pyunder the existing errors and using them accordingly:class QtModuleNotFoundError(ModuleNotFoundError, PythonQtError): """Raised when a Python Qt binding submodule is not installed/supported.""" _msg = 'The {name} module was not found for {binding}.' _msg_extra = '' def __init__(self, *, name, binding=None, msg=None, **msg_kwargs): self.binding = binding msg = msg or f'{self._msg} {self._msg_extra}'.strip() msg = msg.format(name=name, binding=binding, **msg_kwargs) super().__init__(msg, name=name) class QtBindingMissingModuleError(QtModuleNotFoundError): """Raised when a module is not supported by a given binding.""" _msg_extra = 'It is not currently implemented in {binding}.' class QtModuleNotInstalledError(QtModuleNotFoundError): """Raise when a module is supported by the binding, but not installed.""" _msg_extra = 'It must be installed separately' def __init__(self, *, missing_package=None, **superclass_kwargs): self.missing_package = missing_package if missing_package is not None: self._msg_extra += ' as {missing_package}.' super().__init__(missing_package=missing_package, **superclass_kwargs)
Also, a few smaller but common issues (which I made suggestions to fix, except where noted):
- Let's please not use
*imports inmodule_report, as they are considered a very bad practice and make the code hard to understand, introspect and lint, among other reasons. It is true that QtPy uses them internally, but for a very specific purpose—ensuring it picks up the full namespace of the packages it wraps without hardcoding—that isn't the case here. Also, we should use standard, conventional PEP 8 import ordering. - Module-level constants are ALL_CAPS per PEP 8 and standard convention
- Don't forget to add terminating EOLs to the files
- Per PEP 257, docstrings must end with periods and one-line docstrings should be on one line (as they are currently), not with
'''"above and below (I didn't make review comments for this to avoid noise)
Thanks, and looking forward to seeing this in final form!
Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
I made sure module report commits and regular commits never touched because I was expecting to "simply" rebase and drop them. I just don't do these operations often enough to be deeply familiar with them.
I've seen people advocate for what I guess you could call incremental squashing, where a number of similar commits ("added newlines after imports", "added more newlines after imports") get combined, but it's quite likely I'd make a mess doing that, too.
Godspeed. o7 |
|
I just did
I usually do this, either at the end or as I work, but I generally avoid it for contributor PRs since it requires rewriting history on their own branch, and ends up being a lot more complexity than either keeping the commits or auto-squashing. In this case, I was going to do do some commit consolidation to make the rebase easier, but it turned out it wasn't necessary since you were careful to keep your commit separate, well-titled and organized, and there were only a few places I might have squashed things, so I just left things as they were. |
|
In any case, since we're all LGTM and it was pending my review, I'm going to go ahead and merge. Thanks again, everyone, and excited to see your future work @DaelonSuzuka ! |
I used Thank you for cleaning up after me(again).
I can genuinely say it's been a pleasure working with you gentlemen. I'll try to be back soon to discuss more potential improvements. |
Hmm, that all sounds right, although at least for me when I checked out your branch prior to my rebase,
Fantastic, same to you! |
QtWebSockets was not being imported when using PySide6.
Some other modules appear to missing imports as well:
It's possible that some of these were omitted on purpose, but there's no indication of that in the code.
It would also help readability and auditability to standardize the formatting of these files. I'd be willing to do this if the project is open to it.
Additionally, PySide6 has been split into two packages, PySide6-Essentials and PySide6-Addons, and it's probably incorrect behavior to assume that all of PySide6 is present without explicitly checking.